Support PagerDuty API v2 #1795

sebito91 · 2018-02-07T02:17:03Z

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated
Sign CLA (if not already signed)

The PagerDuty API has stepped to v2 and it's a pretty serious refactor of the endpoint. While the v1 endpoint is still supported for some time, it's in deprecated mode and will ultimately be replaced soon(ish).

https://v2.developer.pagerduty.com/docs/events-api-v2
https://v2.developer.pagerduty.com/docs/getting-started

nathanielc

This looks great! Just a few questions below.

nathanielc · 2018-02-07T22:54:54Z

services/pagerduty2/service.go

+	Class         string
+	Component     string
+	Group         string
+	CustomDetails string


Based on the above it looks like this field can be arbitrary JSON, is that correct? If so the type could be map[string]interface{} or possibly just interface{} something and we could add the details JSON as this field.

See the VictorOps handler for how the details can be treated as rich data instead of just a string https://github.com/influxdata/kapacitor/blob/master/services/victorops/service.go#L208. The models.Result type support JSON marshalling so it possibly could be set as the CustomDetails directly.

Was following the existing pagerduty description which actually has this as a standard string. A local commit type set CustomDetails to the map[string]interface{} you called out because that does seem to be the most flexible.

https://github.com/influxdata/kapacitor/blob/master/services/pagerduty/service.go#L155

Do we want to change this one too?

nathanielc · 2018-02-07T22:55:59Z

services/pagerduty2/service.go

+		severity = "critical"
+	case alert.Info:
+		severity = "info"
+	case alert.Error:


How does the alert.Error level get set?

Good catch, this is my reading of the API v2 docs and not the internal representation from the kapacitor side...I'll remove.

sebito91 · 2018-02-07T23:14:55Z

Thanks for the feedback! Still working to get the tests to pass fully before checking once more (comments in-line).

sebito91 · 2018-02-09T23:59:43Z

@nathanielc mind taking another look? I've incorporated your feedback (hopefully)...

sebito91 · 2018-02-10T21:01:22Z

Not sure where this test is failing, but still looking into the fix for this one...

8432 === RUN   TestService_GetConfig
8433 --- FAIL: TestService_GetConfig (0.04s)
8434     service_test.go:1076: expected to get an response on errC
8435 FAIL
8436 FAIL    github.com/influxdata/kapacitor/services/config 1.072s

nathanielc

@sebito91 This is really close. I think we can simplify how the CustomDetails are handled, see my comments below.

Let me know what you think....

nathanielc · 2018-02-27T16:54:40Z

services/pagerduty2/service.go

+	ap.DedupKey = incidentKey
+	ap.EventAction = eventType
+
+	ap.Payload.CustomDetails = make(map[string]interface{})


I think it would be simpler and still just as useful if the CustomDetails where just the data.Result object. The Result object serializes to JSON in a standard format and contains all relevant data to the alert. Will it still work for your use case to just ignore the details string and pass in the Result directly? This way we can avoid any edge cases with trying to unmarshal the details strings since its opaque to the code at this point. Thoughts?

To be clear I am thinking something like this

ap.Payload.CustomDetails = make(map[string]interface{}) ap.Payload.CustomDetails["result"] = data.Result

Since the details strings can only be constructed using the data already in data.Result, then this is a superset of the data being provided. The only question is if the data is still useful if we do it this way.

Ah, that makes sense. WIll update and get back to you ASAP!

nathanielc · 2018-02-27T16:57:30Z

services/pagerduty2/service.go

+		event.State.ID,
+		event.State.Message,
+		event.State.Level,
+		event.State.Details,


As per the above comment we can probably just remove passing the details down if they are not going to be used.

nathanielc

I just went through similar process for OpsGenie(#1823) and in the process I noticed a few small things that would be nice to add to this PR:

Add a pagerduty2 configuration section to the example config in etc/kapacitor/kapacitor.conf
Add integration tests for pagerDuty2 in integrations/streamer_test.go, copy pasting the existing pagerDuty tests is fine. They will need to be updated for the new API structures
pipeline/tick/alert.go needs to support the new pagerDuty2 handlers
Add a test to pipeline/tick/alert_test.go

Thanks for your work on the PR so far!! If you find that you do not have time to finish up some of these items let me know. This is all top of mind for me now and I don't mind finishing up these small housekeeping items since the majority of the work is done.

nathanielc · 2018-03-01T18:18:25Z

server/server_test.go

+				}}
+
+				fmt.Printf("DEBUG -- got: %v\n", got[0].PostData.Payload)
+				fmt.Printf("DEBUG -- exp: %v\n", exp[0].PostData.Payload)


Can you make these either t.Log or remove them?

sebito91 · 2018-03-09T06:32:43Z

@nathanielc apologies for the noise on this PR but I believe I've finally covered the updates you were looking for. Thanks again for your patience!

nathanielc

@sebito91 One last round and this is good to go!

Can you make the two small changes noted below?
Then once they are complete can you rebase and squash the commits into a single commit?

Once that is done I'll merge this. Thanks!

nathanielc · 2018-03-09T17:10:15Z

alert/types.go

@@ -125,6 +125,7 @@ const (
 	Warning
 	Critical
 	maxLevel
+	Error


This is unused correct?

I'll remove this one for now, but it is technically an option for the v2 API.

https://v2.developer.pagerduty.com/docs/send-an-event-events-api-v2

Since I'm not explicitly using it in the service then it's fine to ignore.

Yeah, eventually Kapacitor may support custom alert levels, right now it tries to keep things simple by implementing what is common between most alert services.

Once there is room for customization here, we can enable the pagerduty2 to take advantage of it.

Thanks.

nathanielc · 2018-03-09T17:20:14Z

services/pagerduty2/service.go

+	m := timestamp.Format(time.RFC3339Nano)
+	if timestamp.Nanosecond() == 0 {
+		m = time.Unix(timestamp.Unix(), 1).In(timestamp.Location()).Format(time.RFC3339Nano)
+	}


This is just trying to ensure that the timestamps are correctly padded with zeros? And it does so by adding a single nanosecond? If so there is another way to do this. Go's time formatting docs don't make it super clear but in the format string if you use the digit 0 as a placeholder then it will correctly pad the string to be a fixed width.

From the Go source:

RFC3339Nano = "2006-01-02T15:04:05.999999999Z07:00"

If you use this format instead, then zero padding will be added.

"2006-01-02T15:04:05.000000000Z07:00"

See https://play.golang.org/p/87-3z6vnUnc

sebito91 · 2018-03-09T20:56:18Z

@nathanielc feedback incorporated, PR rebased and squashed!

nathanielc

🎉

Support PagerDuty API v2

sebito91 force-pushed the pd_v2 branch 2 times, most recently from eb99c6f to beea7ae Compare February 7, 2018 06:16

nathanielc suggested changes Feb 7, 2018

View reviewed changes

sebito91 force-pushed the pd_v2 branch from e32dbff to 3759902 Compare February 7, 2018 23:25

timhallinflux mentioned this pull request Feb 8, 2018

Support PagerDuty v2 as part of Kapacitor v1.5 update influxdata/chronograf#2786

Closed

5 tasks

sebito91 force-pushed the pd_v2 branch from ef9fffd to 7eaa952 Compare February 9, 2018 19:41

sebito91 force-pushed the pd_v2 branch 3 times, most recently from 5d19e43 to 04d4ac5 Compare February 10, 2018 17:38

nathanielc suggested changes Feb 27, 2018

View reviewed changes

sebito91 force-pushed the pd_v2 branch 3 times, most recently from 9810aad to f1f4e1c Compare March 1, 2018 00:09

nathanielc suggested changes Mar 1, 2018

View reviewed changes

sebito91 force-pushed the pd_v2 branch 7 times, most recently from 0df83a8 to 874ed58 Compare March 8, 2018 23:58

sebito91 force-pushed the pd_v2 branch from f28a1aa to 06e8fe5 Compare March 9, 2018 05:01

nathanielc suggested changes Mar 9, 2018

View reviewed changes

sebito91 force-pushed the pd_v2 branch from 6b28ebb to 1e8d103 Compare March 9, 2018 20:01

initial commit, support for pagerduty API v2

13ef2f5

sebito91 force-pushed the pd_v2 branch from 3ad5fdd to 13ef2f5 Compare March 9, 2018 20:48

nathanielc approved these changes Mar 9, 2018

View reviewed changes

nathanielc merged commit 13ef2f5 into influxdata:master Mar 9, 2018

nathanielc added a commit that referenced this pull request Mar 9, 2018

Merged pull request #1795 from sebito91/pd_v2

bee81c9

Support PagerDuty API v2

sebito91 deleted the pd_v2 branch March 9, 2018 21:58

nathanielc mentioned this pull request Mar 12, 2018

Feature - Pagerduty API v2 #1358

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support PagerDuty API v2 #1795

Support PagerDuty API v2 #1795

sebito91 commented Feb 7, 2018 •

edited

Loading

nathanielc left a comment

nathanielc Feb 7, 2018

sebito91 Feb 7, 2018

nathanielc Feb 7, 2018

sebito91 Feb 7, 2018

sebito91 commented Feb 7, 2018

sebito91 commented Feb 9, 2018

sebito91 commented Feb 10, 2018

nathanielc left a comment

nathanielc Feb 27, 2018

sebito91 Feb 27, 2018

nathanielc Feb 27, 2018

nathanielc left a comment

nathanielc Mar 1, 2018

sebito91 commented Mar 9, 2018

nathanielc left a comment

nathanielc Mar 9, 2018

sebito91 Mar 9, 2018

nathanielc Mar 9, 2018

nathanielc Mar 9, 2018

sebito91 commented Mar 9, 2018

nathanielc left a comment

Support PagerDuty API v2 #1795

Support PagerDuty API v2 #1795

Conversation

sebito91 commented Feb 7, 2018 • edited Loading

Required for all non-trivial PRs

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebito91 commented Feb 7, 2018

sebito91 commented Feb 9, 2018

sebito91 commented Feb 10, 2018

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebito91 commented Mar 9, 2018

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebito91 commented Mar 9, 2018

nathanielc left a comment

Choose a reason for hiding this comment

sebito91 commented Feb 7, 2018 •

edited

Loading